Driver code done by using Smithy generated code#9
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughApplication bootstrap moved from Spring Boot to a Smithy-based HTTP server. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server as RouteService
participant Activity as RouteActivity
participant Output as ResponseBuilder
Client->>Server: HTTP Request (Create/Delete/Get)
activate Server
Server->>Activity: invoke corresponding operation
activate Activity
Activity->>Output: build default Output
activate Output
Output-->>Activity: Output
deactivate Output
Activity-->>Server: operation result
deactivate Activity
Server-->>Client: HTTP Response
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.gradle.kts (1)
82-89:⚠️ Potential issue | 🟡 MinorPlatform-specific command limits portability.
The
smithyBuildtask usescmd /cwhich only works on Windows. Consider using a cross-platform approach or the Smithy Gradle plugin for better portability.♻️ Suggested cross-platform alternative
tasks.register<Exec>("smithyBuild") { group = "build" description = "Java models codegen from Smithy model." - commandLine("cmd", "/c", "smithy", "build", + commandLine("smithy", "build", "--config", ".\\InAndOut-API-Modelling\\smithy-build.json", "--projection", "java-route-service", "--output", "build/") + if (System.getProperty("os.name").lowercase().contains("windows")) { + commandLine = listOf("cmd", "/c") + commandLine + } }Alternatively, consider using the official Smithy Gradle plugin (
software.amazon.smithy.gradle.smithy-baseorsoftware.amazon.smithy.gradle.smithy-jar) which handles this natively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` around lines 82 - 89, The smithyBuild task is Windows-only because it calls "cmd /c"; change it to a cross-platform implementation by either (A) switching the task to invoke the smithy CLI directly (remove "cmd", " /c" and call commandLine("smithy", ...") or use exec { executable = "smithy"; args = [...] } and add an OS-guard if you must support different executables on Windows vs *nix, or (B) preferably apply the official Smithy Gradle plugin (software.amazon.smithy.gradle.smithy-base or software.amazon.smithy.gradle.smithy-jar) and replace the custom tasks with the plugin's smithy configuration and its built-in tasks (refer to the smithyBuild task name and configure the plugin's projection "java-route-service" and output to "build/").
🧹 Nitpick comments (2)
build.gradle.kts (1)
47-67: Consider removing unused Spring Boot dependencies.The PR transitions from Spring Boot to a Smithy-based HTTP server, yet the Spring Boot plugins (lines 4-6) and all Spring Boot starter dependencies remain. If these are no longer used, they add unnecessary bloat and potential classpath conflicts.
If this is an incremental migration, consider documenting a TODO to remove these dependencies once the transition is complete. Otherwise, remove them now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` around lines 47 - 67, The build includes many Spring Boot starter dependencies (e.g., implementation("org.springframework.boot:spring-boot-starter-jdbc"), implementation("org.springframework.boot:spring-boot-starter-r2dbc"), implementation("org.springframework.boot:spring-boot-starter-restclient"), implementation("org.springframework.boot:spring-boot-starter-security"), implementation("org.springframework.boot:spring-boot-starter-security-oauth2-authorization-server"), implementation("org.springframework.boot:spring-boot-starter-security-oauth2-client"), implementation("org.springframework.boot:spring-boot-starter-session-jdbc"), implementation("org.springframework.boot:spring-boot-starter-webclient"), implementation("org.springframework.boot:spring-boot-starter-webmvc"), implementation("org.springframework.boot:spring-boot-starter-webservices") and corresponding testImplementation entries) that are likely unused after migrating to the Smithy HTTP server; either remove these implementation/testImplementation lines now (after verifying no runtime/test usage) or keep them temporarily but add a clear TODO comment in build.gradle.kts indicating they should be removed once the Spring Boot pieces are fully migrated to avoid classpath bloat and conflicts.src/main/java/com/shopping/inandout/routeservice/activities/CreateRouteActivity.java (1)
9-9: Remove the personal comment from committed source.This line is unrelated to the handler and will just add noise to future diffs. Please keep production files free of personal comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/shopping/inandout/routeservice/activities/CreateRouteActivity.java` at line 9, Remove the unrelated personal comment in CreateRouteActivity.java: delete the stray line "// i love my girlfriend <333333333333" inside the CreateRouteActivity class/file so the source contains only production-relevant comments and code; ensure no other personal or nonfunctional comments remain in the CreateRouteActivity class header or methods (e.g., constructor, handleRequest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.gradle.kts`:
- Around line 74-80: The java.srcDir currently points into the package path
which breaks compilation; update the afterEvaluate -> sourceSets -> main
configuration so that
java.srcDir("build/java-route-service/java-server-codegen") (i.e. the generated
sources root) is used instead of the full package path; modify the java.srcDir
call in the afterEvaluate block that configures sourceSets.main to reference the
generated-source root rather than ".../com/shopping/inandout".
In
`@src/main/java/com/shopping/inandout/routeservice/activities/CreateRouteActivity.java`:
- Around line 12-13: The createRoute method in CreateRouteActivity currently
ignores the input and returns an empty CreateRouteOutput; update
CreateRouteActivity.createRoute to either call the real creation flow (e.g.,
delegate to your route persistence/service method such as
RouteService.createRoute or RouteRepository.save using the provided
CreateRouteInput and RequestContext) and return a populated CreateRouteOutput
reflecting the persisted route, or if the implementation isn’t ready, explicitly
fail by throwing a clear exception (e.g., UnsupportedOperationException) instead
of returning a success; ensure the method uses CreateRouteInput fields to build
the created resource and include any IDs or status in CreateRouteOutput.
In
`@src/main/java/com/shopping/inandout/routeservice/activities/DeleteRouteActivity.java`:
- Around line 10-11: deleteRoute currently always returns success without doing
any deletion; update DeleteRouteActivity.deleteRoute to invoke the actual
deletion flow using the route identifier from DeleteRouteInput (e.g., call the
repository or service delete method such as RouteRepository.deleteById or
RouteService.deleteRoute), handle not-found cases by returning a
failure/exception instead of success, and surface errors/logging via the
provided RequestContext; ensure DeleteRouteOutput reflects the real outcome
(success only when delete succeeded).
In
`@src/main/java/com/shopping/inandout/routeservice/activities/GetRouteActivity.java`:
- Around line 10-11: getRoute currently always returns an empty GetRouteOutput
regardless of input; update GetRouteActivity.getRoute to use the provided
GetRouteInput and RequestContext to perform the actual lookup (e.g., call your
route repository/service with input identifiers) and populate the GetRouteOutput
with the found route data, and if no route is found return or throw a modeled
not-found error (or a not-implemented error if lookup is not yet available)
instead of returning a successful empty body.
In `@src/main/java/com/shopping/inandout/routeservice/RouteService.java`:
- Around line 31-34: The Server is hard-coded to bind to "http://localhost:8888"
in RouteService (Server.builder().endpoints(...)), preventing external/container
access; change it to read the bind address and port from configuration/env
variables (e.g., System.getenv("BIND_ADDR") and System.getenv("PORT") or your
app config provider) and fall back to a non-loopback default like "0.0.0.0" and
port 8888; construct the endpoint URI from those values
(URI.create(String.format("http://%s:%s", bindAddr, port))) and pass that to
Server.builder().endpoints(...) so the service is configurable at deploy time.
- Around line 39-48: Replace the blocking Thread.currentThread().join() approach
and move the graceful shutdown logic into a Runtime.addShutdownHook so JVM
signals trigger server.shutdown().get(); keep the main thread waiting with a
CountDownLatch or CompletableFuture that the shutdown hook completes, and in any
InterruptedException handling (around join or waiting) restore the interrupt
flag (Thread.currentThread().interrupt()) before returning; update places
referencing Thread.currentThread().join(), the catch of InterruptedException,
LOGGER.info("Stopping server...") and server.shutdown().get() to use this
shutdown hook + latch/future pattern so resources are cleaned on JVM
termination.
---
Outside diff comments:
In `@build.gradle.kts`:
- Around line 82-89: The smithyBuild task is Windows-only because it calls "cmd
/c"; change it to a cross-platform implementation by either (A) switching the
task to invoke the smithy CLI directly (remove "cmd", " /c" and call
commandLine("smithy", ...") or use exec { executable = "smithy"; args = [...] }
and add an OS-guard if you must support different executables on Windows vs
*nix, or (B) preferably apply the official Smithy Gradle plugin
(software.amazon.smithy.gradle.smithy-base or
software.amazon.smithy.gradle.smithy-jar) and replace the custom tasks with the
plugin's smithy configuration and its built-in tasks (refer to the smithyBuild
task name and configure the plugin's projection "java-route-service" and output
to "build/").
---
Nitpick comments:
In `@build.gradle.kts`:
- Around line 47-67: The build includes many Spring Boot starter dependencies
(e.g., implementation("org.springframework.boot:spring-boot-starter-jdbc"),
implementation("org.springframework.boot:spring-boot-starter-r2dbc"),
implementation("org.springframework.boot:spring-boot-starter-restclient"),
implementation("org.springframework.boot:spring-boot-starter-security"),
implementation("org.springframework.boot:spring-boot-starter-security-oauth2-authorization-server"),
implementation("org.springframework.boot:spring-boot-starter-security-oauth2-client"),
implementation("org.springframework.boot:spring-boot-starter-session-jdbc"),
implementation("org.springframework.boot:spring-boot-starter-webclient"),
implementation("org.springframework.boot:spring-boot-starter-webmvc"),
implementation("org.springframework.boot:spring-boot-starter-webservices") and
corresponding testImplementation entries) that are likely unused after migrating
to the Smithy HTTP server; either remove these implementation/testImplementation
lines now (after verifying no runtime/test usage) or keep them temporarily but
add a clear TODO comment in build.gradle.kts indicating they should be removed
once the Spring Boot pieces are fully migrated to avoid classpath bloat and
conflicts.
In
`@src/main/java/com/shopping/inandout/routeservice/activities/CreateRouteActivity.java`:
- Line 9: Remove the unrelated personal comment in CreateRouteActivity.java:
delete the stray line "// i love my girlfriend <333333333333" inside the
CreateRouteActivity class/file so the source contains only production-relevant
comments and code; ensure no other personal or nonfunctional comments remain in
the CreateRouteActivity class header or methods (e.g., constructor,
handleRequest).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57d41dad-6eb0-4624-8a3c-c48a6e055970
📒 Files selected for processing (7)
InAndOut-API-Modellingbuild.gradle.ktssrc/main/java/com/shopping/inandout/routeservice/RouteService.javasrc/main/java/com/shopping/inandout/routeservice/RouteServiceApplication.javasrc/main/java/com/shopping/inandout/routeservice/activities/CreateRouteActivity.javasrc/main/java/com/shopping/inandout/routeservice/activities/DeleteRouteActivity.javasrc/main/java/com/shopping/inandout/routeservice/activities/GetRouteActivity.java
💤 Files with no reviewable changes (1)
- src/main/java/com/shopping/inandout/routeservice/RouteServiceApplication.java
| public CreateRouteOutput createRoute(CreateRouteInput input, RequestContext context) { | ||
| return CreateRouteOutput.builder().build(); |
There was a problem hiding this comment.
createRoute currently reports success without creating a route.
Line 13 ignores input and returns a blank CreateRouteOutput, so callers can get a successful create response even though nothing was persisted or delegated downstream. Please wire this handler to the real create path, or fail explicitly until that implementation exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/shopping/inandout/routeservice/activities/CreateRouteActivity.java`
around lines 12 - 13, The createRoute method in CreateRouteActivity currently
ignores the input and returns an empty CreateRouteOutput; update
CreateRouteActivity.createRoute to either call the real creation flow (e.g.,
delegate to your route persistence/service method such as
RouteService.createRoute or RouteRepository.save using the provided
CreateRouteInput and RequestContext) and return a populated CreateRouteOutput
reflecting the persisted route, or if the implementation isn’t ready, explicitly
fail by throwing a clear exception (e.g., UnsupportedOperationException) instead
of returning a success; ensure the method uses CreateRouteInput fields to build
the created resource and include any IDs or status in CreateRouteOutput.
| public DeleteRouteOutput deleteRoute(DeleteRouteInput input, RequestContext context) { | ||
| return DeleteRouteOutput.builder().build(); |
There was a problem hiding this comment.
deleteRoute acknowledges deletes without deleting anything.
Line 11 ignores the request and returns success unconditionally, so clients can believe a route was removed while the backing state remains untouched. This needs to call the real delete path, or fail explicitly until it exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/shopping/inandout/routeservice/activities/DeleteRouteActivity.java`
around lines 10 - 11, deleteRoute currently always returns success without doing
any deletion; update DeleteRouteActivity.deleteRoute to invoke the actual
deletion flow using the route identifier from DeleteRouteInput (e.g., call the
repository or service delete method such as RouteRepository.deleteById or
RouteService.deleteRoute), handle not-found cases by returning a
failure/exception instead of success, and surface errors/logging via the
provided RequestContext; ensure DeleteRouteOutput reflects the real outcome
(success only when delete succeeded).
| public GetRouteOutput getRoute(GetRouteInput input, RequestContext context) { | ||
| return GetRouteOutput.builder().build(); |
There was a problem hiding this comment.
getRoute always returns an empty payload.
Line 11 never uses input to look anything up, so every request will succeed with the same blank response regardless of the requested route. Please delegate to the actual lookup path, or return a modeled not-found/not-implemented error instead of a successful empty body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/shopping/inandout/routeservice/activities/GetRouteActivity.java`
around lines 10 - 11, getRoute currently always returns an empty GetRouteOutput
regardless of input; update GetRouteActivity.getRoute to use the provided
GetRouteInput and RequestContext to perform the actual lookup (e.g., call your
route repository/service with input identifiers) and populate the GetRouteOutput
with the found route data, and if no route is found return or throw a modeled
not-found error (or a not-implemented error if lookup is not yet available)
instead of returning a successful empty body.
| try { | ||
| Thread.currentThread().join(); | ||
| } catch (InterruptedException e) { | ||
| LOGGER.info("Stopping server..."); | ||
| try { | ||
| server.shutdown().get(); | ||
| } catch (InterruptedException | ExecutionException ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java, what happens when Thread.currentThread().join()is called from the current thread, and are JVM SIGINT/SIGTERM shutdowns delivered asInterruptedException on the main thread or through shutdown hooks?
💡 Result:
When Thread.currentThread.join is called from the current thread, the thread blocks indefinitely. The join method waits for the target thread to terminate by repeatedly checking isAlive and calling wait(0) on the thread's monitor while alive. Since the thread is waiting on its own monitor and cannot notify itself (as it's blocked), it deadlocks and never proceeds. This is a common idiom to prevent the main thread from exiting in server applications, allowing daemon threads to continue running until an external shutdown signal is received. JVM SIGINT (Ctrl+C) and SIGTERM shutdowns do not deliver InterruptedException to the main thread (or any application threads). These signals trigger the JVM's shutdown sequence: shutdown hooks are executed (if registered via Runtime.addShutdownHook), then the JVM halts. InterruptedException is only thrown due to explicit Thread.interrupt calls, not OS signals. The JVM handles SIGINT/SIGTERM internally without interrupting application threads; instead, it runs shutdown hooks for cleanup. Daemon threads continue during shutdown but are terminated when the JVM halts.
Citations:
- 1: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/lang/Thread.html
- 2: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Thread.html
- 3: https://www.baeldung.com/java-thread-join
- 4: https://stackoverflow.com/questions/34629023/use-of-thread-currentthread-join-in-java
- 5: https://stackoverflow.com/questions/5999595/thread-join-on-itself
- 6: https://stackoverflow.com/questions/32315589/what-happens-when-the-jvm-is-terminated
- 7: https://stackoverflow.com/questions/26856242/interruptedexception-what-causes-it
- 8: https://docs.oracle.com/javase/7/docs/webnotes/tsg/TSG-VM/html/signals.html
- 9: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/lang/Runtime.html
🏁 Script executed:
fd -t f RouteService.javaRepository: InAndOut-Stack/InAndOut-Route-Service
Length of output: 147
🏁 Script executed:
cat -n src/main/java/com/shopping/inandout/routeservice/RouteService.java | sed -n '35,55p'Repository: InAndOut-Stack/InAndOut-Route-Service
Length of output: 624
Thread.currentThread().join() will not trigger graceful shutdown on JVM termination.
Calling Thread.currentThread().join() blocks indefinitely and relies on InterruptedException to unblock. However, normal JVM shutdown signals (Ctrl+C, SIGTERM) do not interrupt application threads—they trigger shutdown hooks instead. This means server.shutdown().get() in the catch block will never execute during graceful shutdown, leaving server resources uncleaned.
Register a shutdown hook with Runtime.addShutdownHook() for graceful shutdown logic, and use a CountDownLatch or CompletableFuture to keep the main thread alive until shutdown is triggered. If catching InterruptedException, restore the interrupt flag before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/shopping/inandout/routeservice/RouteService.java` around
lines 39 - 48, Replace the blocking Thread.currentThread().join() approach and
move the graceful shutdown logic into a Runtime.addShutdownHook so JVM signals
trigger server.shutdown().get(); keep the main thread waiting with a
CountDownLatch or CompletableFuture that the shutdown hook completes, and in any
InterruptedException handling (around join or waiting) restore the interrupt
flag (Thread.currentThread().interrupt()) before returning; update places
referencing Thread.currentThread().join(), the catch of InterruptedException,
LOGGER.info("Stopping server...") and server.shutdown().get() to use this
shutdown hook + latch/future pattern so resources are cleaned on JVM
termination.
1 participant
The goal of this PR is to be able to build the project and run it locally. The expected functional output is the ability to respond with 200 to simple curl commands in the terminal.
Summary by CodeRabbit
New Features
Chores